readline: fix issue with newline-less last line #47317
Merged
+85
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines. The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well. There is some subtlety in this functioning, however. If the last line contains no new lines, then `lastIndex` will be the start of the last line, and `kInsertString` will be called from that point. If it does contain a new line, `lastIndex` will be equal to `s.length`, so the slice will be the empty string. Fixes: nodejs#47305
nodejs-github-bot
added
needs-ci
|
I haven't included a test because I'm not sure what it should be. Since I found this after the fix to the previous infinite loop bug I found (#46731), I'm hesitant to just copy and paste the multi-line test and remove the trailing newline. I also am about to go home for the night but wanted to get this up so nobody wastes their time trying to figure out #47305, if this does actually fix it. Further, with the amount of issues around this, I'm not sure if someone wants a closer eye on what's happening and why these bugs are getting through. |
|
A test is definitely needed here, otherwise the issue is going to come back eventually. |
The repl test probably isn't strictly necessary, but this area's tests has been lacking sufficiently varying cases which have caused a few bugs to be missed, so it seems reasonable to include.
|
I added a repl test which is virtually identical to the multiline test that already existed. It might be overkill, but I figure just in case. The readline test checks the minimal issue I posted previously. Both tests fail without the update to readline/interface.js. |
aduh95
reviewed
Apr 2, 2023
There was a problem hiding this comment.
Thanks, LGTM if tests are passing. Two comments:
- Could you add a comment in
test/fixtures/repl-load-multiline-no-trailing-newline.jsthat the missing EOL is on purpose (sure it's in the name of the file, but that can easily be missed). - Can you run the linter to fix all the reported style nits?
aduh95
approved these changes
Apr 2, 2023
aduh95
added
author ready
github-actions
bot
removed
the
request-ci
This was referenced Apr 3, 2023
github-actions
bot
removed
the
request-ci
Theo-Steiner
reviewed
Apr 4, 2023
26 tasks
aduh95
added
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 9decb70 |
targos
pushed a commit
that referenced
this pull request
May 2, 2023
The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines. The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well. There is some subtlety in this functioning, however. If the last line contains no new lines, then `lastIndex` will be the start of the last line, and `kInsertString` will be called from that point. If it does contain a new line, `lastIndex` will be equal to `s.length`, so the slice will be the empty string. Fixes: #47305 PR-URL: #47317 Reviewed-By: Antoine du Hamel <[email protected]>
danielleadams
pushed a commit
that referenced
this pull request
Jul 6, 2023
The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines. The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well. There is some subtlety in this functioning, however. If the last line contains no new lines, then `lastIndex` will be the start of the last line, and `kInsertString` will be called from that point. If it does contain a new line, `lastIndex` will be equal to `s.length`, so the slice will be the empty string. Fixes: #47305 PR-URL: #47317 Reviewed-By: Antoine du Hamel <[email protected]>
MoLow
pushed a commit
to MoLow/node
that referenced
this pull request
Jul 6, 2023
The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines. The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well. There is some subtlety in this functioning, however. If the last line contains no new lines, then `lastIndex` will be the start of the last line, and `kInsertString` will be called from that point. If it does contain a new line, `lastIndex` will be equal to `s.length`, so the slice will be the empty string. Fixes: nodejs#47305 PR-URL: nodejs#47317 Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The logic for reading lines was slightly flawed, in that it assumed there would be a final new line. It handled the case where there are no new lines, but this then broke if there were some new lines.
The fix in logic is basically removing the special case where there are no new lines by changing it to always read the final line with no new lines. This works because if a file contains no new lines, the final line is the first line, and all is well.
There is some subtlety in this functioning, however. If the last line contains no new lines, then
lastIndexwill be the start of the last line, andkInsertStringwill be called from that point. If it does contain a new line,lastIndexwill be equal tos.length, so the slice will be the empty string.Fixes: #47305